High — behavioral regression on the Thrift hot path (verified against PR head SHA + main).

Pre-PR fetchChunkInternal sequence:

1. failIfClosed()
2. waitUntilReady()
3. getResultHandler() ← may fetch result-metadata RPC
4. failIfClosed() ← critical mid-flight check
5. setTimeout(0) macrotask yield
6. resultHandler.fetchNext() ← data RPC (CloudFetch URL download / Arrow batch)
7. failIfClosed()

Post-PR sequence: steps 3–6 now happen inside ThriftOperationBackend.fetchChunk as one atomic block with no failIfClosed check between them. The only intervening macrotask yield (setTimeout(0) at lib/thrift-backend/ThriftOperationBackend.ts:143-145) sits inside that block.

A concurrent cancel()/close() arriving during the yield window will no longer short-circuit the subsequent fetchNext() data RPC. CloudFetch presigned-URL downloads can take seconds, making this observable rather than theoretical.

Suggested fix: add an isClosed: () => boolean callback to IOperationBackend.fetchChunk's options, and have ThriftOperationBackend.fetchChunk check it after setTimeout(0) returns. Alternatively split the metadata-prep phase into a separate backend method so the facade can re-check between them.

Found by: security, performance (independently).

_{Posted by code-review-squad · /full-review}

Medium — locks the cross-backend interface to a Thrift wire shape.

IOperationBackend.waitUntilReady(options?: WaitUntilReadyOptions) is the cross-backend interface, but WaitUntilReadyOptions (imported from ./IOperation) types callback as (progress: TGetOperationStatusResp) => unknown — a Thrift-generated wire type.

Every non-Thrift backend impl must either:

• depend on the thrift IDL just to construct TGetOperationStatusResp, or
• import synthesizeThriftStatus from lib/utils/thriftWireSynthesis.ts per-poll, or
• skip the user's callback, silently breaking a public contract.

For a Rust kernel polling every ~100 ms during a 10-min query, that's 6,000 synthesized fake Thrift structs purely to satisfy a JS callback signature.

Suggested fix: introduce a separate IOperationBackendWaitOptions { callback?: (status: OperationStatus) => unknown } on the backend interface. DBSQLOperation owns the user's Thrift-typed callback and adapts to/from the neutral one at the facade boundary.

Easier to fix now (one caller, in this PR's own facade) than after the SEA/napi impls land.

Found by: architecture, devils-advocate, security, language.

_{Posted by code-review-squad · /full-review}

Medium — latent correctness bug; bites once any non-Thrift backend is wired in.

synthesizeOkStatus() (line 12) returns { statusCode: TStatusCode.SUCCESS_STATUS } as TStatus unconditionally, and is wired into the status field of the synthesized TGetOperationStatusResp regardless of OperationState.

Existing user code that calls Status.assert(resp.status) after await op.status() against a non-Thrift backend will see SUCCESS_STATUS even when the operation finished in ERROR_STATE or CANCELED_STATE. The lossy-by-design JSDoc comment covers fields like taskStatus/numModifiedRows but does NOT cover status — status is meant to reflect the RPC, but here it's being used as a stand-in for the operation outcome that user code already checks.

Today this is dead code (SEA is a throw-stub); the moment any non-Thrift backend's status() is reachable, error states will silently look successful in the legacy IOperation.status() return value.

Suggested fix: map operation-state to a real TStatus.statusCode:

• OperationState.Failed → ERROR_STATUS (and carry errorMessage/sqlState)
• OperationState.Cancelled → ERROR_STATUS
• other states → SUCCESS_STATUS

Trivial, prevents a latent correctness gap.

Found by: agent-compat, devils-advocate.

_{Posted by code-review-squad · /full-review}

Medium — coverage gap that masks the latent correctness bug in M2.

No test exercises client.connect({ useSEA: true }) end-to-end. The synthesize helpers in this file have no companion tests/unit/utils/thriftWireSynthesis.test.ts. Both DBSQLOperation.status() and DBSQLOperation.getMetadata() have the instanceof ThriftOperationBackend synthesize fallback as dead code from the test suite's perspective.

Any enum-mapping drift (e.g. the M2 issue above) would not be caught by CI.

Suggested fix: add a unit test file tests/unit/utils/thriftWireSynthesis.test.ts covering:

• operationStateToThrift: all 7 enum values
• resultFormatToThrift: all 3 enum values
• synthesizeThriftStatus: confirm errorMessage/sqlState are preserved; confirm hasResultSet round-trips; confirm state: Failed ⇒ non-SUCCESS TStatus (after M2 fix)
• synthesizeThriftResultSetMetadata: confirm schema, arrowSchema, isStagingOperation, lz4Compressed are preserved

And one new test in DBSQLClient.test.ts covering the useSEA: true failure path (using as any cast to bypass the InternalConnectionOptions non-exported type).

Found by: test, ops, agent-compat (independently flagged different aspects).

_{Posted by code-review-squad · /full-review}

Low — latent crash hazard. Pre-refactor the Thrift EventEmitter typing guaranteed error: Error. Post-refactor payload is unknown and the cast is unchecked. If a future backend ever emits a non-Error value through this path (e.g. a string, which EventEmitter.emit('error', ...) permits), error.stack || error.name + ': ' + error.message will throw TypeError. The try/catch only protects this.emit, not the logger.log line.

• Suggested fix:

  const error = payload instanceof Error ? payload : new Error(String(payload));

_{Posted by code-review-squad · /full-review}

Low — confusing diagnostic on a partially-initialized path.

With useSEA: true, SeaBackend.connect() always throws (M0 stub), but this.backend is already assigned. A subsequent openSession() passes the !this.backend guard at line 298 and reaches SeaBackend.openSession(), which throws "SEA backend not implemented yet" instead of "DBSQLClient: not connected".

• Suggested fix: either (a) assign this.backend after a successful connect(), or (b) try/catch around await this.backend.connect(options) and null this.backend on failure.

The IBackend.close() JSDoc explicitly mentions "MUST be safe to call on a partially-initialized backend (i.e. after a failed connect())" — but that contract only covers close(), not the openSession-after-failed-connect path.

_{Posted by code-review-squad · /full-review}

Low — asymmetric with operationStateToThrift above.

operationStateToThrift (lines 16-34) uses an explicit OperationState.Unknown member, making the switch exhaustively-checkable by TS. resultFormatToThrift (lines 36-47) just has a bare default: return COLUMN_BASED_SET. Today the default is dead because ResultFormat is a closed 3-value enum, but if it's extended for SEA (e.g. a new format), the fallback would silently route results through JsonResultHandler and produce garbled data with no error.

• Suggested fix: drop the default, or throw new HiveDriverError(...). Match operationStateToThrift's pattern.

_{Posted by code-review-squad · /full-review}

Low — agent/IDE discoverability.

The JSDoc says "Prefer DBSQLOperation.getResultMetadata in new code" but does NOT use the @deprecated tag. TypeScript's @deprecated is the canonical signal that IDEs (strikethrough), tsc, ESLint plugins, and agentic codegen pick up. Free-text "Prefer X" won't.

• Suggested fix:

  /**
   * @deprecated Use {@link DBSQLOperation.getResultMetadata}; this method synthesizes Thrift-only fields as `undefined` on non-Thrift backends.
   */

_{Posted by code-review-squad · /full-review}

Low — back-compat re-export should signal Thrift-only.

numberToInt64 traffics in node-int64 (a Thrift wire type) and has no place in a backend-neutral facade — it's intentionally re-exported only to preserve back-compat for existing external consumers. Add @deprecated to the re-export and a // Thrift-only JSDoc on the symbol itself, so SDK consumers see the deprecation in their IDE / .d.ts.

_{Posted by code-review-squad · /full-review}

Low — readability / maintenance.

Pre-refactor, handleResponse did one job (re-check open-flag after the backend call returned, because server-side close doesn't error out). The PR removes the helper and inlines a failIfClosed bracket around every backend call — 11 sites with the same shape, no name attached to the "before AND after" semantics.

• Suggested fix:

  private async runBackend<T>(fn: () => Promise<T>): Promise<T> {
    await this.failIfClosed();
    const result = await fn();
    await this.failIfClosed();
    return result;
  }

  Then call sites become return this.wrapOperation(await this.runBackend(() => this.backend.getCatalogs(request)));. Cuts boilerplate and re-attaches a name to the contract.

_{Posted by code-review-squad · /full-review}